-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Check for invalid characters prior to rename #5193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
On Windows platforms, rename operations containing ':' are problematic since the operation succeeds, but creates an unintended "alternate data stream" that gives the user the impression their file contents have been deleted. This change performs cursory validation of the destination name on platforms in which issues can occur. Currently, non-Windows systems don't require this level of validation. Fixes jupyter#5190
|
|
||
| for char in invalid_chars: | ||
| if char in path: | ||
| raise web.HTTPError(400, u"Path '{}' contains invalid characters {} relative to its platform. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about Path '{}' contains characters that are invalid for the filesystem. Path names on this filesystem cannot contain any of the following characters: {}.? I'm trying to make the message simpler, though it seems a little less actionable too (though in the validation method, it's not clear exactly who is calling us and what the recovery procedure should be...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just define it as a string instead of a tuple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invalid_chars = ':><*?|"'
jasongrout
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good, though I can't test on a mac very well. I wrote an inline comment about the error message for consideration.
f0f627d to
2d3d347
Compare

On Windows platforms, rename operations containing ':' are problematic
since the operation succeeds, but creates an unintended "alternate data
stream" that gives the user the impression their file contents have been
deleted. This change performs cursory validation of the destination
name on platforms in which issues can occur. Currently, non-Windows
systems don't require this level of validation.
When an invalid character is encountered during the rename operation, a message similar to the following will occur:

Fixes #5190